Add DefaultEndpointResolver to the orchestrator #2577

ysaito1001 · 2023-04-13T15:59:09Z

Motivation and Context

This PR adds DefaultEndpointResolver, a default implementer of the EndpointResolver trait, to the orchestrator.

Description

Roughly speaking, the endpoint-related work is currently done by make_operation and SmithyEndpointStage, where make_operation constructs endpoint params and applies an endpoint resolver and the SmithyEndpointStage later updates the request header based on the resolved endpoint. In the orchestrator world, that work is handled by DefaultEndpointResolver::resolve_and_apply_endpoint.

The way endpoint parameters and an endpoint prefix are made available to DefaultEndpointResolver::resolve_and_apply_endpoint is done by interceptors because they may require pieces of information only available in an operation input struct. The place that has access to both an operation input struct and a config bag happens to be an interceptor.

There are two interceptors <Operation>EndpointParamsInterceptor and <Operation>EndpointParamsFinalizerInterceptor per operation. We pass around endpoint params builder through interceptors to allow it to be configured with more information at a later point; An end point parameters builder is first initialized within the ServiceRuntimePlugin with the field values obtained from the service config, and is stored in a config bag. The finalizer interceptor "seals" the builder and creates the actual endpoint params before it is passed to DefaultEndpointResolver::resolve_and_apply_endpoint. These interceptors implement read_before_execution because they need to access an operation input before it gets serialized (otherwise, context.input() would return a None).

Testing

Replaced StaticUriEndpointResolver with DefaultEndpointResolver in sra_test and verified the test continued passing.

UPDATE: The test currently fails in the main branch (it returns a None when extracting SigV4OperationSigningConfig from the config bag in OverrideSigningTimeInterceptor, hence unwrap fails), and by merging the main branch this PR no longer passes the test, but it does not add new failures either.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

This commit adds `DefaultEndpointResolver` to the orchestrator. The type implements the `EndpointResolver` trait and takes over the work previously done by `make_operation` and `SmithyEndpointStage`.

This commit places `DefaultEndpointResolver` and endpoint params builder in the config bag from within `ServiceRuntimePlugin`. Notice we put a builder, not endpoint params, because full construction of endpoint params requires additional values to be set from an operation input, which will later be handled by an interceptor.

This commit generates interceptors for configuring endpoint params in the orchestrator. There are two interceptors: one is to update params builder with operation specific information, e.g. a bucket name, and the other is to convert the params builder to the actual endpoint params by calling `.build`.

This commit adds a codegen decorator to register interceptors to `OperationRuntimePlugin`.

This commit replaces `StaticUriEndpointResolver` with `DefaultEndpointResolver` in `sra_test` to ensure the test continues to work end-to-end.

github-actions · 2023-04-13T20:05:42Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)
Client Test (ignoring whitespace)
No codegen difference in the Server Test
No codegen difference in the Server Test Python
No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

This commit allows the `clippy::useless_conversion` only when `enableNewSmithyRuntime` is true. When we stop rendering an endpoint prefix in `make_operation` after completingly switching over to the orchestrator world, we can remove the attribute.

github-actions · 2023-04-13T22:27:04Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)
Client Test (ignoring whitespace)
No codegen difference in the Server Test
No codegen difference in the Server Test Python
No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

jdisanti

Missed this PR since I was out last week. I have some comments around errors.

.../smithy/rust/codegen/client/smithy/endpoint/generators/EndpointParamsInterceptorGenerator.kt

rust-runtime/aws-smithy-runtime-api/src/client/endpoints.rs

jdisanti · 2023-04-18T00:16:24Z

rust-runtime/aws-smithy-runtime-api/src/client/endpoints.rs

+                    HeaderName::from_str(header_name).map_err(|err| {
+                        ResolveEndpointError::message("invalid header name")
+                            .with_source(Some(err.into()))
+                    })?,
+                    HeaderValue::from_str(value).map_err(|err| {
+                        ResolveEndpointError::message("invalid header value")
+                            .with_source(Some(err.into()))
+                    })?,


Should these errors even be possible if the endpoint config is valid?

I am not entirely sure. I've decided to preserve the existing error handling logic from the original PR for the sake of porting. I don't see how an endpoint resolver comes into being after a series of config steps in the orchestrator world is all that different from the old world, so maybe it makes sense to keep the existing error handling for now (we can later turn them into non-recoverable errors, if we want).

This commit incorporates the following feedback: #2577 (comment) #2577 (comment) #2577 (comment) #2577 (comment)

This commit addresses #2577 (comment)

## Motivation and Context This PR incorporates post-merge feedback left in #2577. ---- _By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice._ --------- Co-authored-by: Yuki Saito <awsaito@amazon.com>

This commit ports fixes made in #2592, which addresses the following #2577 (comment)

## Motivation and Context This PR adds `DefaultEndpointResolver`, a default implementer of the [EndpointResolver](https://github.com/awslabs/smithy-rs/blob/1e27efe05fe7b991c9f9bbf3d63a297b2dded334/rust-runtime/aws-smithy-runtime-api/src/client/orchestrator.rs#L182-L184) trait, to the orchestrator. ## Description Roughly speaking, the endpoint-related work is currently done by `make_operation` and `SmithyEndpointStage`, where `make_operation` constructs endpoint params and applies an endpoint resolver and the `SmithyEndpointStage` later updates the request header based on the resolved endpoint. In the orchestrator world, that work is handled by `DefaultEndpointResolver::resolve_and_apply_endpoint`. The way endpoint parameters and an endpoint prefix are made available to `DefaultEndpointResolver::resolve_and_apply_endpoint` is done by interceptors because they _may_ require pieces of information only available in an operation input struct. The place that has access to both an operation input struct and a config bag happens to be an interceptor. There are two interceptors `<Operation>EndpointParamsInterceptor` and `<Operation>EndpointParamsFinalizerInterceptor` per operation. We pass around endpoint params _builder_ through interceptors to allow it to be configured with more information at a later point; An end point parameters builder is first initialized within the `ServiceRuntimePlugin` with the field values obtained from the service config, and is stored in a config bag. The finalizer interceptor "seals" the builder and creates the actual endpoint params before it is passed to `DefaultEndpointResolver::resolve_and_apply_endpoint`. These interceptors implement `read_before_execution` because they need to access an operation input before it gets serialized (otherwise, `context.input()` would return a `None`). ## Testing Replaced `StaticUriEndpointResolver` with `DefaultEndpointResolver` in `sra_test` and verified the test continued passing. UPDATE: The test currently fails in the `main` branch (it returns a `None` when extracting `SigV4OperationSigningConfig` from the config bag in `OverrideSigningTimeInterceptor`, hence `unwrap` fails), and by merging the `main` branch this PR no longer passes the test, but it does not add new failures either. ---- _By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice._ --------- Co-authored-by: Yuki Saito <awsaito@amazon.com>

## Motivation and Context This PR incorporates post-merge feedback left in #2577. ---- _By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice._ --------- Co-authored-by: Yuki Saito <awsaito@amazon.com>

## Motivation and Context This PR adds `DefaultEndpointResolver`, a default implementer of the [EndpointResolver](https://github.com/awslabs/smithy-rs/blob/1e27efe05fe7b991c9f9bbf3d63a297b2dded334/rust-runtime/aws-smithy-runtime-api/src/client/orchestrator.rs#L182-L184) trait, to the orchestrator. ## Description Roughly speaking, the endpoint-related work is currently done by `make_operation` and `SmithyEndpointStage`, where `make_operation` constructs endpoint params and applies an endpoint resolver and the `SmithyEndpointStage` later updates the request header based on the resolved endpoint. In the orchestrator world, that work is handled by `DefaultEndpointResolver::resolve_and_apply_endpoint`. The way endpoint parameters and an endpoint prefix are made available to `DefaultEndpointResolver::resolve_and_apply_endpoint` is done by interceptors because they _may_ require pieces of information only available in an operation input struct. The place that has access to both an operation input struct and a config bag happens to be an interceptor. There are two interceptors `<Operation>EndpointParamsInterceptor` and `<Operation>EndpointParamsFinalizerInterceptor` per operation. We pass around endpoint params _builder_ through interceptors to allow it to be configured with more information at a later point; An end point parameters builder is first initialized within the `ServiceRuntimePlugin` with the field values obtained from the service config, and is stored in a config bag. The finalizer interceptor "seals" the builder and creates the actual endpoint params before it is passed to `DefaultEndpointResolver::resolve_and_apply_endpoint`. These interceptors implement `read_before_execution` because they need to access an operation input before it gets serialized (otherwise, `context.input()` would return a `None`). ## Testing Replaced `StaticUriEndpointResolver` with `DefaultEndpointResolver` in `sra_test` and verified the test continued passing. UPDATE: The test currently fails in the `main` branch (it returns a `None` when extracting `SigV4OperationSigningConfig` from the config bag in `OverrideSigningTimeInterceptor`, hence `unwrap` fails), and by merging the `main` branch this PR no longer passes the test, but it does not add new failures either. ---- _By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice._ --------- Co-authored-by: Yuki Saito <awsaito@amazon.com>

## Motivation and Context This PR incorporates post-merge feedback left in #2577. ---- _By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice._ --------- Co-authored-by: Yuki Saito <awsaito@amazon.com>

## Motivation and Context - #2577 added a `Debug` bound on `ResolveEndpoint`, but this was a semver-incompatible change ## Description This removes that bound but re-adds a Debug implementation in areas that we need them ## Testing - CI - [ ] Backport this commit to main ---- _By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice._

## Motivation and Context Port #2630 to the `main` branch. ## Description Adding `Debug` bound on `ResolveEndpoint` in #2577 was a breaking change. Unfortunately, the semvar check that ran in that PR barked at a different thing (i.e. barked at moving `StaticUriEndpointResolver` and `DefaultEndpointResolver` from `aws-smithy-runtime-api` to `aws-smithy-runtime`). Eventually the breaking change got merged to the `main` branch. This PR reverts the change in question and implements the `Debug` trait for `ResolveEndpoint` in a semvar non-breaking way. ## Testing - [ ] Passed tests in CI ---- _By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice._ Co-authored-by: Yuki Saito <awsaito@amazon.com>

Port #2630 to the `main` branch. Adding `Debug` bound on `ResolveEndpoint` in #2577 was a breaking change. Unfortunately, the semvar check that ran in that PR barked at a different thing (i.e. barked at moving `StaticUriEndpointResolver` and `DefaultEndpointResolver` from `aws-smithy-runtime-api` to `aws-smithy-runtime`). Eventually the breaking change got merged to the `main` branch. This PR reverts the change in question and implements the `Debug` trait for `ResolveEndpoint` in a semvar non-breaking way. - [ ] Passed tests in CI ---- _By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice._ Co-authored-by: Yuki Saito <awsaito@amazon.com>

ysaito1001 added 5 commits April 13, 2023 09:33

Add default endpoint resolver to the orchestrator

1e2efa7

This commit adds `DefaultEndpointResolver` to the orchestrator. The type implements the `EndpointResolver` trait and takes over the work previously done by `make_operation` and `SmithyEndpointStage`.

Register interceptors for configuring endpoint params

3973f0f

This commit adds a codegen decorator to register interceptors to `OperationRuntimePlugin`.

Use DefaultEndpointResolver in sra_test

ae25016

This commit replaces `StaticUriEndpointResolver` with `DefaultEndpointResolver` in `sra_test` to ensure the test continues to work end-to-end.

ysaito1001 requested review from a team as code owners April 13, 2023 15:59

ysaito1001 requested review from jdisanti and crisidev April 13, 2023 15:59

ysaito1001 added the needs-sdk-review label Apr 13, 2023

Merge branch 'main' into ysaito/default-ep-resolver-in-orc

f3b3d50

ysaito1001 added 2 commits April 13, 2023 17:01

Derive Debug for types implementing ResolveEndpoint

0b4087a

Velfi removed the needs-sdk-review label Apr 14, 2023

Velfi approved these changes Apr 14, 2023

View reviewed changes

ysaito1001 added this pull request to the merge queue Apr 17, 2023

Merged via the queue into main with commit 7cb9d6a Apr 17, 2023

ysaito1001 deleted the ysaito/default-ep-resolver-in-orc branch April 17, 2023 21:25

jdisanti reviewed Apr 18, 2023

View reviewed changes

ysaito1001 added a commit that referenced this pull request Apr 18, 2023

Avoid using InterceptorError in Interceptor impl

10a1967

This commit incorporates the following feedback: #2577 (comment) #2577 (comment) #2577 (comment) #2577 (comment)

ysaito1001 added a commit that referenced this pull request Apr 18, 2023

Move concrete endpoint resolver types to runtime-api

8219785

This commit addresses #2577 (comment)

ysaito1001 mentioned this pull request Apr 18, 2023

Follow up on DefaultEndpointResolver in the orchestrator #2592

Merged

ysaito1001 added a commit that referenced this pull request Apr 21, 2023

Port fixes made in #2592

5e48760

This commit ports fixes made in #2592, which addresses the following #2577 (comment)

rcoh mentioned this pull request Apr 25, 2023

Fix debug bound on 0.55.x #2630

Merged

1 task

ysaito1001 mentioned this pull request Apr 26, 2023

Port fix for debug bound on ResolveEndpoint #2636

Merged

1 task

ysaito1001 mentioned this pull request Apr 27, 2023

Code-gen setters on endpoint ParamsBuilder within runtime plugins/interceptors #2644

Closed

ysaito1001 mentioned this pull request May 8, 2023

add: tests to ensure all interceptors are run if an error occurs in one #2664

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add DefaultEndpointResolver to the orchestrator #2577

Add DefaultEndpointResolver to the orchestrator #2577

ysaito1001 commented Apr 13, 2023 •

edited

Loading

github-actions bot commented Apr 13, 2023

github-actions bot commented Apr 13, 2023

jdisanti left a comment

jdisanti Apr 18, 2023

ysaito1001 Apr 18, 2023

Add DefaultEndpointResolver to the orchestrator #2577

Add DefaultEndpointResolver to the orchestrator #2577

Conversation

ysaito1001 commented Apr 13, 2023 • edited Loading

Motivation and Context

Description

Testing

github-actions bot commented Apr 13, 2023

github-actions bot commented Apr 13, 2023

jdisanti left a comment

Choose a reason for hiding this comment

jdisanti Apr 18, 2023

Choose a reason for hiding this comment

ysaito1001 Apr 18, 2023

Choose a reason for hiding this comment

ysaito1001 commented Apr 13, 2023 •

edited

Loading